Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PLANET-7647 Enforce consistency on default favicon #2507

Merged
merged 1 commit into from
Jan 22, 2025
Merged

Conversation

mleray
Copy link
Contributor

@mleray mleray commented Jan 21, 2025

Description

See PLANET-7647

We remove the option to customize it in the site identity settings, and we need to run a migration to remove existing values

Testing

1. Setting removal

If you go to Appearance > Customize > Site Identity you should no longer be able to change the site icon.

Before the removal you would see this:

Screenshot 2025-01-21 at 10 37 33

2. Migration script

You can run it on local with npx wp-env run cli wp p4-run-activator

Before the migration, we had a custom site icon:

Screenshot 2025-01-21 at 11 09 24

After the migration, you should now see the default site icon:

Screenshot 2025-01-21 at 11 09 33

@mleray mleray self-assigned this Jan 21, 2025
planet-4 added a commit to greenpeace/planet4-test-titan that referenced this pull request Jan 21, 2025

Verified

This commit was signed with the committer’s verified signature.
rentallect Curt Tudor
/unhold db2c7bb6-ff3e-4724-bce8-a489944395ff
planet-4 added a commit to greenpeace/planet4-test-titan that referenced this pull request Jan 21, 2025

Verified

This commit was signed with the committer’s verified signature.
rentallect Curt Tudor
/unhold dd69ef98-2642-443c-8125-ee64fb0a81a9

Verified

This commit was signed with the committer’s verified signature.
rentallect Curt Tudor
We remove the option to customize it in the site identity settings, and we need to run a migration to remove existing values
planet-4 added a commit to greenpeace/planet4-test-titan that referenced this pull request Jan 21, 2025

Verified

This commit was signed with the committer’s verified signature.
rentallect Curt Tudor
/unhold e137cd9c-7d5a-457a-94bf-e3ae73c7fe66
@mleray mleray added the Review label Jan 21, 2025
planet-4 added a commit to greenpeace/planet4-test-titan that referenced this pull request Jan 21, 2025

Verified

This commit was signed with the committer’s verified signature.
rentallect Curt Tudor
/unhold 4fd16a74-3aac-4022-b8b9-f491e69bbdb9
@greenpeace greenpeace deleted a comment from planet-4 Jan 21, 2025
planet-4 added a commit to greenpeace/planet4-test-tavros that referenced this pull request Jan 21, 2025

Verified

This commit was signed with the committer’s verified signature.
rentallect Curt Tudor
/unhold 987691d2-91db-4c08-a6e0-2aca9c68f5c1
@planet-4
Copy link
Contributor

Test instance is ready 🚀

🌑 tavros | admin | blocks report | CircleCI | composer-local.json

⌚ 2025.01.21 11:26:41

@mleray mleray marked this pull request as ready for review January 21, 2025 12:06
@mleray mleray requested review from comzeradd, a team and Osong-Michael and removed request for a team January 21, 2025 12:06
Copy link
Contributor

@Osong-Michael Osong-Michael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mleray after the migration, we loose the favicon for the page when on the admin dashboard and not the site itself, is this the expected behaviour?

Test Instance
Screenshot 2025-01-21 at 13 47 48

Local Instance
Screenshot 2025-01-21 at 13 48 44

PS: Does not seem like a big deal since only editors use this.

@mleray
Copy link
Contributor Author

mleray commented Jan 21, 2025

Hey @mleray after the migration, we loose the favicon for the page when on the admin dashboard and not the site itself, is this the expected behaviour?

@Osong-Michael I do not see this on the instance 🤔 The icon is updated in the admin interface too

Screenshot 2025-01-21 at 14 49 27

Edit: I do see the issue on local, but only in certain admin pages, not all of them... weird!

@mleray mleray requested a review from Osong-Michael January 21, 2025 16:09
Copy link
Contributor

@Osong-Michael Osong-Michael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me 👍

@mleray
Copy link
Contributor Author

mleray commented Jan 22, 2025

I would like @comzeradd to have a look since we have issues on local... on the instance everything seems to be fine though

@comzeradd
Copy link
Member

comzeradd commented Jan 22, 2025

I would like @comzeradd to have a look since we have issues on local... on the instance everything seems to be fine though

Not sure where the admin panel gets its favicon. I don't see anything relevant on the html markup.

On admin panel, I don't see the icon at all indeed when at local.

On the test instance, I see the right one while in frontend. But I still see the wrong one (with the colored background) in the admin panel even on private/incognito mode.

@mleray mleray merged commit 8af4dfa into main Jan 22, 2025
15 checks passed
@mleray mleray deleted the default-favicon branch January 22, 2025 13:09
comzeradd added a commit that referenced this pull request Feb 20, 2025

Verified

This commit was signed with the committer’s verified signature.
rentallect Curt Tudor
Ref: https://jira.greenpeace.org/browse/PLANET-7647

---

Follow up from #2507,
there is one more additional option besides the one we disabled.
comzeradd added a commit that referenced this pull request Feb 20, 2025
Ref: https://jira.greenpeace.org/browse/PLANET-7647

---

Follow up from #2507,
there is one more additional option besides the one we disabled.
comzeradd added a commit that referenced this pull request Feb 21, 2025
Ref: https://jira.greenpeace.org/browse/PLANET-7647

---

Follow up from #2507,
there is one more additional option besides the one we disabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants